⚡ Spark: Dialog backdrop click and native event sync - #66
Conversation
- Added `closeOnBackdropClick` prop to `Dialog` component (default: `false`). - Implemented state synchronization with native `close` event to handle ESC key and other native closing triggers. - Updated documentation and unit tests to cover new functionality. - Optimized `useEffect` by leveraging native event listeners for state management.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe Dialog component gains a new ChangesDialog closeOnBackdropClick Feature
Sequence DiagramsequenceDiagram
participant User
participant Dialog as Dialog Element
participant ClickHandler as closeOnBackdropClick Handler
participant CloseHandler as onClose Handler
participant State as Dialog State
User->>Dialog: click on backdrop
Dialog->>ClickHandler: trigger onClick
ClickHandler->>ClickHandler: check if click outside bounds
ClickHandler->>Dialog: call close()
Dialog->>Dialog: remove open attribute
Dialog->>Dialog: dispatch close event
Dialog->>CloseHandler: native close event
CloseHandler->>State: set open = false
CloseHandler->>CloseHandler: call onClose callback
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/components/Dialog/index.tsx`:
- Around line 70-73: handleNativeClose currently closes the dialog and calls
onClose but is declared inline, risking a stale closure over the onClose prop;
update handleNativeClose to either (a) wrap it in useCallback with onClose and
setOpen in its dependency array so it always calls the latest onClose, or (b)
store the latest onClose in a ref (e.g., onCloseRef.current) and have
handleNativeClose call that ref so the handler need not be re-created — change
the implementation referencing the handleNativeClose function, the setOpen call,
and the onClose prop accordingly.
In `@README.md`:
- Line 258: Update the README table entry for the closeOnBackdropClick prop to
note that it only applies when behavior is set to 'modal'; reference the prop
name closeOnBackdropClick and the behavior prop (behavior="modal") so readers
know this is modal-only (matching the JSDoc on the Dialog interface and the
implementation check in Dialog component).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e6094e48-7835-478a-a2fa-b114d10ebb4a
📒 Files selected for processing (4)
.axioma/spark.mdREADME.mdlib/components/Dialog/index.test.tsxlib/components/Dialog/index.tsx
| const handleNativeClose = () => { | ||
| setOpen(false) | ||
| onClose?.() | ||
| } |
There was a problem hiding this comment.
Stale closure risk: handleNativeClose may capture an outdated onClose callback.
The handleNativeClose function is defined inline and will capture the onClose prop from the render where it was created. If the parent component passes a new onClose function on subsequent renders (e.g., due to inline arrow functions or changing dependencies), the event handler will continue invoking the stale callback until the component re-renders and re-attaches the handler.
This is a common pitfall in React event handlers that capture props or state.
🔄 Proposed fix: wrap in useCallback or use a ref
Option 1: Wrap in useCallback (simpler)
+const handleNativeClose = useCallback(() => {
+ setOpen(false)
+ onClose?.()
+}, [onClose])
-const handleNativeClose = () => {
- setOpen(false)
- onClose?.()
-}Option 2: Use a ref to always call the latest callback (no re-render on prop change)
+const onCloseRef = useRef(onClose)
+useEffect(() => {
+ onCloseRef.current = onClose
+}, [onClose])
+
+const handleNativeClose = useCallback(() => {
+ setOpen(false)
+ onCloseRef.current?.()
+}, [])
-const handleNativeClose = () => {
- setOpen(false)
- onClose?.()
-}Option 2 is preferred if you want to avoid re-attaching the event listener when onClose changes, though for dialog close events this overhead is negligible.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleNativeClose = () => { | |
| setOpen(false) | |
| onClose?.() | |
| } | |
| const handleNativeClose = useCallback(() => { | |
| setOpen(false) | |
| onClose?.() | |
| }, [onClose]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/components/Dialog/index.tsx` around lines 70 - 73, handleNativeClose
currently closes the dialog and calls onClose but is declared inline, risking a
stale closure over the onClose prop; update handleNativeClose to either (a) wrap
it in useCallback with onClose and setOpen in its dependency array so it always
calls the latest onClose, or (b) store the latest onClose in a ref (e.g.,
onCloseRef.current) and have handleNativeClose call that ref so the handler need
not be re-created — change the implementation referencing the handleNativeClose
function, the setOpen call, and the onClose prop accordingly.
1 participant
💡 What: Enhanced the
Dialogcomponent with two key features:closeOnBackdropClickprop).<dialog>element's internal state.🎯 Why:
<dialog>elements can be closed via the ESC key or other browser-specific triggers. Without syncing with the nativecloseevent, the React state (andonClosecallback) would remain out of sync.📦 What it adds:
closeOnBackdropClickprop toDialogProps.closeevents.🚀 How to use:
♻️ Deps Free: Verified, only uses React and native Browser APIs.
🎨 Harmony: Follows existing
Dialogpatterns and maintains backward compatibility.PR created automatically by Jules for task 4682436667730776089 started by @galiprandi
Summary by CodeRabbit
New Features
closeOnBackdropClickprop to automatically close the dialog when users click outside its content area (disabled by default).Documentation